Mongodb support for partial updates. - #380
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 43 minutes and 18 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR refactors the MongoDB connector's Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
connectors/mongo/connector_test.go (1)
68-77: Test helpers swallow/abuseDecode— small robustness fix.
assertDocignores theDecodeerror; ifFindOnefails for a reason other than "document missing",reswill be nil and theassert.Equalmessage will misleadingly point at a value mismatch instead of the underlying driver error.assertNoDocpassesniltoDecode, which is fragile (behavior when the document unexpectedly exists depends on driver internals). Prefer.Err()to check the not-found condition directly.♻️ Suggested change
func assertDoc(t *testing.T, col *mongo.Collection, expected map[string]string) { var res map[string]string - col.FindOne(t.Context(), bson.M{"_id": expected["_id"]}).Decode(&res) + require.NoError(t, col.FindOne(t.Context(), bson.M{"_id": expected["_id"]}).Decode(&res)) assert.Equal(t, expected, res) } func assertNoDoc(t *testing.T, col *mongo.Collection, id string) { - err := col.FindOne(t.Context(), bson.M{"_id": id}).Decode(nil) - assert.ErrorIs(t, err, mongo.ErrNoDocuments) + assert.ErrorIs(t, col.FindOne(t.Context(), bson.M{"_id": id}).Err(), mongo.ErrNoDocuments) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@connectors/mongo/connector_test.go` around lines 68 - 77, assertDoc currently ignores the error from FindOne().Decode which can hide driver errors and produce misleading assertions; change assertDoc to capture and assert.NoError on the Decode call (e.g., err := col.FindOne(...).Decode(&res); assert.NoError(t, err)) before asserting equality with expected, and in assertNoDoc replace the Decode(nil) usage with checking the FindOne(...).Err() result (e.g., err := col.FindOne(...).Err(); assert.ErrorIs(t, err, mongo.ErrNoDocuments)) to reliably detect the "not found" condition; update the functions named assertDoc and assertNoDoc accordingly.connectors/mongo/conn.go (2)
1010-1025: DuplicatedidKeysmap build.
stripIdFields(line 925) already constructed the sameidKeysset fromidFilter. Rebuilding it here is redundant; you could either hoist the set above theswitchor expose a helper that returns it fromstripIdFields. Minor, but removes duplication and one allocation per partial update.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@connectors/mongo/conn.go` around lines 1010 - 1025, The idKeys map is rebuilt redundantly inside the partial-unset block; reuse the idKeys computed by stripIdFields instead of allocating a new map. Modify stripIdFields to either return the idKeys set alongside its current result or compute idKeys once before the switch that handles partial updates, then reference that idKeys in the partial-unset logic (the code using idFilter and update.GetPartialUpdateUnset()). Remove the inner idKeys construction to eliminate the duplicate allocation.
979-984: Dead assignment in theisNewbranch.
lastTypeis only consulted in the!isNewbranches of each case below; whenisNewis true the code always appends tomodelsregardless oflastType. ThelastType = update.GetType()assignment is never read. Either drop it, or simplify by only tracking "was the last op a partial update" inseen(amap[string]bool), which also removes theadiomv1.UpdateTypeimport-coupling on what is really a boolean signal.♻️ Minor simplification
- seen := map[string]adiomv1.UpdateType{} + lastIsPartial := map[string]bool{} for i := len(updates) - 1; i >= 0; i-- { update := updates[i] idFilter, idKey, err := c.buildIdFilter(update) if err != nil { return nil, false, err } - lastType, found := seen[idKey] - isNew := !found - if isNew { - seen[idKey] = update.GetType() - lastType = update.GetType() - } + last, found := lastIsPartial[idKey] + isNew := !found + if isNew { + lastIsPartial[idKey] = update.GetType() == adiomv1.UpdateType_UPDATE_TYPE_PARTIAL_UPDATE + last = lastIsPartial[idKey] + }Then replace
lastType == adiomv1.UpdateType_UPDATE_TYPE_PARTIAL_UPDATEwithlast.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@connectors/mongo/conn.go` around lines 979 - 984, The dead assignment sets lastType = update.GetType() in the isNew branch but lastType is only used for non-new paths; remove this unused assignment and simplify the seen map to track only whether the last op was a partial update (e.g., change seen from map[string]adiomv1.UpdateType to map[string]bool or a new seenPartial map keyed by idKey), update the branch that sets seen[idKey] to store a boolean (true if update.GetType() == adiomv1.UpdateType_UPDATE_TYPE_PARTIAL_UPDATE), and replace all uses of lastType == adiomv1.UpdateType_UPDATE_TYPE_PARTIAL_UPDATE with the boolean flag (e.g., last) so models appending logic remains correct and the adiomv1 type coupling is removed; also drop the now-unread lastType variable and its assignment in the isNew branch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@connectors/mongo/conn.go`:
- Around line 924-958: stripIdFields currently swallows errors and returns the
original raw BSON, which can leave immutable _id/shard keys and cause Mongo
update failures; change stripIdFields to return (bson.Raw, error) instead of
just bson.Raw, return a non-nil error when raw.Elements() or
bson.Marshal(filtered) fails (and return nil, nil if filtering removes all
fields), and update the PARTIAL_UPDATE caller to handle the error (log/abort the
bulk op or skip the patch) rather than assigning the unchanged raw into $set;
reference the stripIdFields function and the PARTIAL_UPDATE path where its
result is used so callers can properly react to failures.
---
Nitpick comments:
In `@connectors/mongo/conn.go`:
- Around line 1010-1025: The idKeys map is rebuilt redundantly inside the
partial-unset block; reuse the idKeys computed by stripIdFields instead of
allocating a new map. Modify stripIdFields to either return the idKeys set
alongside its current result or compute idKeys once before the switch that
handles partial updates, then reference that idKeys in the partial-unset logic
(the code using idFilter and update.GetPartialUpdateUnset()). Remove the inner
idKeys construction to eliminate the duplicate allocation.
- Around line 979-984: The dead assignment sets lastType = update.GetType() in
the isNew branch but lastType is only used for non-new paths; remove this unused
assignment and simplify the seen map to track only whether the last op was a
partial update (e.g., change seen from map[string]adiomv1.UpdateType to
map[string]bool or a new seenPartial map keyed by idKey), update the branch that
sets seen[idKey] to store a boolean (true if update.GetType() ==
adiomv1.UpdateType_UPDATE_TYPE_PARTIAL_UPDATE), and replace all uses of lastType
== adiomv1.UpdateType_UPDATE_TYPE_PARTIAL_UPDATE with the boolean flag (e.g.,
last) so models appending logic remains correct and the adiomv1 type coupling is
removed; also drop the now-unread lastType variable and its assignment in the
isNew branch.
In `@connectors/mongo/connector_test.go`:
- Around line 68-77: assertDoc currently ignores the error from FindOne().Decode
which can hide driver errors and produce misleading assertions; change assertDoc
to capture and assert.NoError on the Decode call (e.g., err :=
col.FindOne(...).Decode(&res); assert.NoError(t, err)) before asserting equality
with expected, and in assertNoDoc replace the Decode(nil) usage with checking
the FindOne(...).Err() result (e.g., err := col.FindOne(...).Err();
assert.ErrorIs(t, err, mongo.ErrNoDocuments)) to reliably detect the "not found"
condition; update the functions named assertDoc and assertNoDoc accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 593996a5-6f4a-4e63-a82a-bcab3a34f783
📒 Files selected for processing (5)
connectors/mongo/conn.goconnectors/mongo/conn_unit_test.goconnectors/mongo/connector_test.goconnectors/util/util.goconnectors/util/util_test.go
32b3fc6 to
37935f8
Compare
37935f8 to
fc09988
Compare
Summary by CodeRabbit
Bug Fixes
New Features